Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integrated SPARQL Query; addresses SPARQL-issue-1 #3

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

waximabbax
Copy link
Contributor

@waximabbax waximabbax commented Nov 7, 2023

For #1
integrated SPARQL Query; addresses SPARQL-issue#1

@waximabbax waximabbax force-pushed the feature/SPARQL-issue#1 branch 5 times, most recently from ce474ea to e57ea97 Compare November 7, 2023 13:41
@JeroenDeDauw
Copy link
Member

How we at Professional Wiki typically do dependency injection in MediaWiki extensions:

Ultimately what we are after is well-designed code that is easy to test and understand and is reasonably robust against framework changes.

For this PR that means:

  • No need to touch HookHandlers or extension.json
  • You need a new PHP class with public function runQuery( string $sparql ): array. Since we have full control over this class, you can require all config and collaborators in the constructor. You can also develop and test this class without dealing with MediaWiki stuff.
  • You could construct the new PHP class directly in SparqlLua (the "entry point"), and thus do all access to MediaWikiServices::getInstance() there. There should be no global access beyond that point (ie in the new class)
  • If you use the FileFetcher mini-interface in your class, you can use the already existing test doubles in your tests. This is what External Content does. You can see how it constructs an instance of FileFetcher at https://github.com/ProfessionalWiki/ExternalContent/blob/master/src/EmbedExtensionFactory.php#L105-L112.

@JeroenDeDauw
Copy link
Member

JeroenDeDauw commented Nov 7, 2023

Having looked at my notes in #1 again, I just realized that if we go with the POST request approach, then using FileFetcher won't work, as it only has a URL parameter. Which means we either use HttpRequestFactory directly, or create a new wrapper, if HttpRequestFactory proves too difficult to deal with in tests.

I cannot recall running into this before. @malberts do you know of a place where we solved making POST requests in MediaWiki testable?

@malberts
Copy link
Contributor

malberts commented Nov 7, 2023

I'm not aware of any place where we're doing external requests (POST or otherwise) as part of our own code.

@waximabbax waximabbax force-pushed the feature/SPARQL-issue#1 branch 5 times, most recently from c437d9f to 4cfd8fe Compare November 7, 2023 19:50
@waximabbax waximabbax changed the title WIP: integrated SPARQL Query; addresses SPARQL-issue#1 integrated SPARQL Query; addresses SPARQL-issue#1 Nov 7, 2023
@waximabbax waximabbax marked this pull request as ready for review November 7, 2023 21:28
@waximabbax
Copy link
Contributor Author

Manual Test Evidence

TEST_Sparql_issue.1.mp4

src/SparqlLua.php Outdated Show resolved Hide resolved
@waximabbax waximabbax force-pushed the feature/SPARQL-issue#1 branch 3 times, most recently from e727d1b to 922a3bc Compare November 8, 2023 09:01
src/SparqlLua.php Outdated Show resolved Hide resolved
src/SparqlLua.php Outdated Show resolved Hide resolved
src/SparqlLua.php Outdated Show resolved Hide resolved
src/SparqlLuaQueryRunner.php Outdated Show resolved Hide resolved
src/SparqlLuaQueryRunner.php Outdated Show resolved Hide resolved
src/SparqlLuaQueryRunner.php Outdated Show resolved Hide resolved
src/SparqlLuaQueryRunner.php Outdated Show resolved Hide resolved
src/SparqlLuaQueryRunner.php Outdated Show resolved Hide resolved
src/SparqlLuaQueryRunner.php Outdated Show resolved Hide resolved
@waximabbax waximabbax changed the title integrated SPARQL Query; addresses SPARQL-issue#1 integrated SPARQL Query; addresses SPARQL-issue-1 Nov 8, 2023
@waximabbax waximabbax force-pushed the feature/SPARQL-issue#1 branch 7 times, most recently from 7e1ed8e to f581c64 Compare November 8, 2023 15:53
@JeroenDeDauw JeroenDeDauw merged commit 19ac5c7 into master Nov 8, 2023
12 checks passed
@JeroenDeDauw JeroenDeDauw deleted the feature/SPARQL-issue#1 branch November 8, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants